feat(generators): add --normalize-prefixes for conventional prefix names in every generator - #4

🔍 Adversarial Review — PR #4

Summary

Solid feature with thorough test coverage, but contains a confirmed race condition between Phase 1 and Phase 2 of normalize_graph_prefixes() that can silently swap namespace URIs (HTTPS → HTTP) for schema.org. The cross-PR conflict with PR #1 (well_known_prefix_map() defined differently in both) also needs a merge strategy.

🐛 Bugs & Issues

1. Phase 2 overwrites Phase 1 normalization for HTTP/HTTPS variants (Confirmed)

When a schema declares sdo: https://schema.org/ and the metamodel runtime injects schema: http://schema.org/, the following sequence occurs:

• Phase 1 correctly rebinds sdo → schema: https://schema.org/ (user's HTTPS)
• After Phase 1, the graph still contains schema1: http://schema.org/ (orphaned runtime HTTP binding)
• Phase 2 sees schema1: http://schema.org/, looks up wk["http://schema.org/"] = "schema", checks collision ("schema" not in schema_prefixes — user declared sdo, not schema), and rebinds schema: http://schema.org/ with override=True, replace=True
• This silently replaces the Phase 1 HTTPS binding with the HTTP variant

Reproduced locally with rdflib 7.x:

from rdflib import Graph, Namespace, URIRef
g = Graph()
g.bind('schema', Namespace('http://schema.org/'))      # metamodel
g.namespace_manager.bind('sdo', URIRef('https://schema.org/'))  # user

# Phase 1
g.bind('schema', Namespace('https://schema.org/'), override=True, replace=True)
# → schema: https://schema.org/ ✅, schema1: http://schema.org/ (orphan)

# Phase 2
g.bind('schema', Namespace('http://schema.org/'), override=True, replace=True)
# → schema: http://schema.org/ ❌  (HTTPS binding silently lost!)

Fix: Phase 2 should check whether std_pfx is already bound in the graph (not just schema_prefixes) before rebinding:

# In Phase 2, before rebinding:
current_ns_for_std = dict(graph.namespaces()).get(std_pfx)
if current_ns_for_std and str(current_ns_for_std) != ns_str:
    continue  # std_pfx already claimed by Phase 1 for a different URI

2. test_sdo_normalised_to_schema may be passing for the wrong reason

The test asserts pfx["schema"] == "https://schema.org/", but if Phase 2 overwrites this with HTTP, the test should fail. If it passes, it may be because the metamodel's emit_prefixes don't include schema for the test schema, meaning the HTTP binding is never injected — hiding the bug in production schemas that DO trigger both bindings. The test should explicitly set up both HTTP and HTTPS bindings to exercise Phase 2 properly.

⚠️ Concerns

3. Cross-PR conflict with PR #1 — well_known_prefix_map() defined differently

PR #4's static approach is explicitly designed to solve a problem with PR #1's dynamic approach (rdflib defaults changed between 6.x and 7.x). These will conflict on merge. Recommendation: merge PR #4 first, or rebase PR #1 to use PR #4's static map.

4. Cross-PR conflict — flag forwarding in jsonldgen.py

PR #4 forwards both normalize_prefixes AND deterministic to the embedded ContextGenerator:

for flag in ("normalize_prefixes", "deterministic"):
    if hasattr(self, flag):
        context_kwargs.setdefault(flag, getattr(self, flag))

PR #1 adds deterministic handling in a different location (post-serialization in end_schema). PR #4's forwarding of deterministic creates a dependency on PR #1's field existing. The hasattr guard prevents a crash, but semantically PR #4 shouldn't forward a flag it doesn't define. Either:

• Remove "deterministic" from the forwarding loop (PR feat(generators): add --normalize-prefixes for conventional prefix names in every generator #4 doesn't own this flag), or
• Document the intentional coupling with PR feat(generators): add --deterministic flag with hybrid RDFC-1.0 + rdflib serialization #1 and establish merge order

5. PR title says fix: but commit says feat: — which is it?

The commit message uses feat(generators): while the PR title uses fix(generators):. This is a new opt-in flag, which is a feature — feat: is correct per conventional commits.

6. Static map maintenance burden

The _WELL_KNOWN_PREFIX_MAP needs manual updates when rdflib adds new curated defaults. The test_matches_rdflib_defaults test catches additions but not removals. Consider adding a reverse check: entries in the static map that are NOT in rdflib's defaults could indicate stale entries.

🧪 Test Coverage Assessment

Strong coverage (18 tests across 7 classes):

• ✅ OWL, SHACL, JSON-LD context normalization (sdo→schema, dce→dc)
• ✅ HTTP/HTTPS schema.org variants
• ✅ Collision detection with warnings (caplog)
• ✅ Cross-generator consistency
• ✅ Static map integrity and copy safety
• ✅ JSONLDGenerator flag forwarding
• ✅ Flag-off backward compatibility

Gaps:

• ❌ No test for normalize_prefixes + exclude_imports interaction
• ❌ No test for empty schema (zero prefixes) — is _prefix_remap.clear() safe?
• ❌ No test that explicitly sets up both HTTP and HTTPS runtime bindings to exercise Phase 2 overwrite (the scenario from Bug feat(generators): add --deterministic flag with hybrid RDFC-1.0 + rdflib serialization #1)
• ❌ No test for add_prefix being called before __post_init__ completes (defensive, but _prefix_remap access would crash)
• ❌ CURIE remapping test (test_normalize_prefixes_curie_remapping) only checks that Person @id starts with schema: — doesn't verify full_name slot's @id uses schema:name instead of sdo:name

📋 Fix Plan

1. Fix Phase 2 overwrite bug — add a graph-binding check before rebinding in Phase 2 to prevent overwriting Phase 1's normalization
2. Add regression test — explicitly set up a graph with both schema: http://schema.org/ and sdo: https://schema.org/, run normalize, and assert the HTTPS binding survives
3. Remove "deterministic" from forwarding loop — or gate it behind hasattr only (already done) but add a comment explaining the cross-PR dependency
4. Fix PR title — change fix: to feat: for consistency with the commit
5. Add empty-schema test — verify no crash when schema has zero prefixes
6. Add exclude_imports interaction test — verify normalization works when imports are excluded
7. Strengthen CURIE remapping test — assert that slot URIs (not just class URIs) are remapped in the context

✅ What's Good

• Excellent documentation: the 3-case handling in visit_schema is well-commented with case numbers
• Defensive _prefix_remap initialization: correctly placed before super().__post_init__() with clear comment explaining why
• Collision detection: warning on skip is the right behavior — no silent data loss
• MappingProxyType for the static map: prevents accidental mutation
• well_known_prefix_map() returns a copy: callers can't corrupt the canonical map
• Phase 2 concept is sound: cleaning up runtime-injected orphan bindings is the right idea, it just needs the overwrite guard
• Cross-generator consistency test: ensures OWL, SHACL, and JSON-LD agree — excellent
• Backward compatibility: flag defaults to off, existing behavior preserved